Conversation
dd6cb82 to
4e5b186
Compare
|
Ah...we just had the same/similar in stackabletech/stackable-utils#130 It's an annoying trap. |
in this context, it is not a huge deal - from what I checked it does not interfere with OOM based restarts or something like that - but getting the actual exit codes may be nice and should be an improvement. |
maltesander
moved this from Development: Waiting for Review
to Development: In Review
in Stackable Engineering
maltesander
left a comment
maltesander
left a comment
There was a problem hiding this comment.
A couple of things. Almost all downstream products require changes:
- https://github.com/stackabletech/trino-operator/blob/77bcf6da54ac49fb2d896388f791611f79984a50/rust/operator-binary/src/controller/build/command.rs#L145
- https://github.com/stackabletech/nifi-operator/blob/4d6cea98741fb34c29d28ae4e13931dcb317d817/rust/operator-binary/src/controller/build/resource/statefulset.rs#L404
Will abort under set -e with non-zero code, neither create_vector_shutdown_file_command or other lines after that will be executed (e.g. the vector shutdown file etc.). So || product_exit_code=$? should be added and rolled out with an operator-rs release properly?
And i think a similar problem for git-sync containers
It could make sense to stop that test script duplication (i think its in over 10 places) and e.g. use insta (already in the repo) as dev/test dependency? Something like:
insta::assert_snapshot!(
serde_yaml::to_string(&git_sync_resources.git_sync_containers.first()).unwrap()
);
f3a4fee to
64793f9
Compare
|
@maltesander sorry for the long time it took me to work through your comments properly. I have now extended the changelog (126afa0) to mark this as breaking and added additional tests for checking if commands after product crash + Next, I am looking at insta. |
64793f9 to
e973e6b
Compare
f36e2e0 to
d8849ca
Compare
|
@maltesander I changed the tests to use insta - see 704b60a It simplifies a bunch of things (some clippy lints can go away) but on the other hand we have additional files. Take a look whether you like it, otherwise I can also drop this commit. |
This branch has not been deployed
3 participants
Description
since
set -eis the last part ofwait_for_terminationit always has exit code 0. With this, it returns the exit code of the process it is waiting for instead.[ "${term_child_pid}" ]blows up when the child pid is not set yet, so gets changed to[ -n "${term_child_pid:-}" ].Definition of Done Checklist
Author
Reviewer
Acceptance